Skip to content

Guard maps in legacy handler #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Sayan- merged 2 commits into from
Oct 22, 2025
Merged

Guard maps in legacy handler #9

Sayan- merged 2 commits into from
Oct 22, 2025

Conversation

@Sayan-
Copy link
Collaborator

@Sayan- Sayan- commented Oct 22, 2025 •
edited
Loading

Overview

While doing some local stress testing I actually hit the case where concurrent map writes caused us to crash out:

[neko] fatal error: concurrent map writes

[neko] goroutine 5642 [running]:
[neko] internal/runtime/maps.fatal({0x13955ef?, 0x0?})
	/usr/local/go/src/runtime/panic.go:1046 +0x20
[neko] internal/runtime/maps.(*Map).Delete(0x40001157a0, 0x1260ce0, 0x40021b7438)
	/usr/local/go/src/internal/runtime/maps/map.go:652 +0x48
[neko] github.com/m1k1o/neko/server/internal/http/legacy.(*session).destroy(0x40032ea5a0)
	/src/internal/http/legacy/session.go:207 +0xd0
[neko] github.com/m1k1o/neko/server/internal/http/legacy.(*LegacyHandler).Route.func1({0xffff4dcc1c60, 0x400213b8c0}, 0x4002860140)
	/src/internal/http/legacy/handler.go:212 +0x794

Fixing for reliability!

I also had gpt-5 scan the internal implementation for any similar cases, resulting in c667c74

Testing

CI

@Sayan- Sayan- requested review from hiroTamada and rgarcia October 22, 2025 17:42
@mesa-dot-dev
Copy link

mesa-dot-dev bot commented Oct 22, 2025 •
edited
Loading

Mesa Description

Overview

While doing some local stress testing I actually hit the case where concurrent map writes caused us to crash out:

[neko] fatal error: concurrent map writes

[neko] goroutine 5642 [running]:
[neko] internal/runtime/maps.fatal({0x13955ef?, 0x0?})
	/usr/local/go/src/runtime/panic.go:1046 +0x20
[neko] internal/runtime/maps.(*Map).Delete(0x40001157a0, 0x1260ce0, 0x40021b7438)
	/usr/local/go/src/internal/runtime/maps/map.go:652 +0x48
[neko] github.com/m1k1o/neko/server/internal/http/legacy.(*session).destroy(0x40032ea5a0)
	/src/internal/http/legacy/session.go:207 +0xd0
[neko] github.com/m1k1o/neko/server/internal/http/legacy.(*LegacyHandler).Route.func1({0xffff4dcc1c60, 0x400213b8c0}, 0x4002860140)
	/src/internal/http/legacy/handler.go:212 +0x794

This PR fixes the fatal race condition by adding a sync.Mutex to the LegacyHandler. This ensures that all read and write operations on the sessionIPs and bannedIPs maps are thread-safe, preventing the server from crashing under concurrent requests.

Testing

CI

Description generated by Mesa. Update settings

mesa-dot-dev[bot]
mesa-dot-dev bot reviewed Oct 22, 2025
View reviewed changes
Copy link

@mesa-dot-dev mesa-dot-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performed full review of 560fac5...ac19476

Analysis

  1. While the PR addresses concurrent map access with mutexes, there's no mention of performance impact analysis for high-traffic scenarios where lock contention could become an issue
  2. The solution seems to focus only on mutex-based synchronization rather than considering alternative concurrency patterns (like sync.Map or read-write locks) that might offer better performance characteristics
  3. There's no indication of comprehensive testing to verify the fix works under actual concurrent load conditions

Tip

Help

Configure your agents

Mesa Docs

Slash Commands:

  • /review - Request a full code review
  • /review latest - Review only changes since the last review
  • /describe - Generate PR description. This will update the PR body or issue comment depending on your configuration
  • /help - Get help with Mesa commands and configuration options

2 files reviewed | 0 comments | Edit Agent Settings

hiroTamada
hiroTamada approved these changes Oct 22, 2025
View reviewed changes
Copy link

@hiroTamada hiroTamada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great great catch!

@Sayan- Sayan- merged commit 5073274 into master Oct 22, 2025
4 checks passed
@Sayan- Sayan- deleted the sayan/life-happens-fast branch October 22, 2025 18:46
@Sayan- Sayan- mentioned this pull request Oct 22, 2025
Merged
Sayan- added a commit to kernel/kernel-images that referenced this pull request Oct 22, 2025
@Sayan-
Bump neko (#83)
01dc993 
Pulls in kernel/neko#8
kernel/neko#9

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> Update chromium-headful Dockerfile to use neko base image
3.0.8-v1.3.0.
> 
> - **Docker**:
> - Bump base image in `images/chromium-headful/Dockerfile` from
`ghcr.io/onkernel/neko/base:3.0.8-v1.1.0` to `3.0.8-v1.3.0`.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
e6afb32. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgarcia rgarcia rgarcia approved these changes

@hiroTamada hiroTamada hiroTamada approved these changes

+1 more reviewer

@mesa-dot-dev mesa-dot-dev[bot] mesa-dot-dev[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Sayan- @rgarcia @hiroTamada